Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(pluck): fix pluck's catch-all signature for better type safety #5192

Merged
merged 3 commits into from Jan 20, 2020
Merged

fix(pluck): fix pluck's catch-all signature for better type safety #5192

merged 3 commits into from Jan 20, 2020

Conversation

Lonelind
Copy link
Contributor

@Lonelind Lonelind commented Dec 23, 2019

Description: Change pluck's catch-all signature that is causing troubles when the
passed string is not a key in the argument object.

Related issue (if exists): #5188

Change pluck's catch-all signature that is causing troubles when the
passed string is not a key in the argument object.

Issue: #5188
@Lonelind
Copy link
Contributor Author

Just happened to see that there is a line that was added 7 months ago that does virtually the same but with one core difference: if you use R there, it will fail the same way as it was already but it will happen with the seventh argument passed. The pluck operator will resolve type by itself returning what was specified in result (it will fall down to unknown only if it was any there). Passing unknown every time is safer. You will need to check the type, yes but it will not say that everything is okay even if it isn't.

Correct the signtaure to be accepted by tests and to return unknown in
general
@Lonelind
Copy link
Contributor Author

Lonelind commented Dec 23, 2019

Also, I've added a test case that checks type inference:

it('should not infer type from the variable if key doesn\'t exist', () => {
  const a: Observable<number> = of({ name: 'abc' }).pipe(pluck('xyz')); // $ExpectError
});

With the old implementation there's no error emitting and the pluck signature gets number as the R. This fix will return unknown there, and the types don't match since, that is an expected behaviour.

Copy link
Collaborator

@cartant cartant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks.

@cartant cartant added target: master & patch TS Issues and PRs related purely to TypeScript issues labels Jan 19, 2020
@benlesh
Copy link
Member

benlesh commented Jan 20, 2020

This can't go into patch, because it will break people's builds if they're violating the new constraint. However, it's a good addition to master.

@benlesh benlesh merged commit e0c5b7c into ReactiveX:master Jan 20, 2020
@Lonelind Lonelind deleted the fix-pluck-signature branch January 21, 2020 09:18
martinsik pushed a commit to martinsik/rxjs that referenced this pull request Feb 15, 2020
…eactiveX#5192)

* fix(pluck): fix pluck's catch-all signature for better type safety

Change pluck's catch-all signature that is causing troubles when the
passed string is not a key in the argument object.

Issue: ReactiveX#5188

* fix(pluck): fix signature and tests

Correct the signtaure to be accepted by tests and to return unknown in
general

* fix(pluck): add test case to check type inference
@lock lock bot locked as resolved and limited conversation to collaborators Feb 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
TS Issues and PRs related purely to TypeScript issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants